Repository navigation
Conversation
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR adds always-on Electron titlebar Back and Forward controls and a new route-history/sessionStorage tracker, along with related titlebar layout behavior. Because it introduces user-facing desktop functionality and production navigation state behavior rather than an off-by-default option, human review is warranted. You can add or adjust custom eligibility rules. Learn more. |
|
Warning Review limit reachedOnly developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing. Next included review available in 22 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (5)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe router now tracks the furthest navigation index, and Electron sidebar titlebars provide Back and Forward controls. Sidebar layout and branding behavior also adjust for backdrop placement and platform-specific widths. ChangesElectron navigation controls
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant getRouter
participant trackNavigationHistory
participant RouterHistory
participant SessionStorage
participant NavigationHistoryControls
getRouter->>trackNavigationHistory: Start tracking RouterHistory
trackNavigationHistory->>RouterHistory: Subscribe to navigation changes
trackNavigationHistory->>SessionStorage: Read or persist furthest index
NavigationHistoryControls->>RouterHistory: Read forward availability
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The desktop navigation controls appear ready to merge after normal checks; no actionable blocking issue is established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The buttons use existing navigation capabilities without introducing new destinations or privileges. Remaining uncertainty concerns history restoration and lifecycle behavior, rather than a demonstrated security weakness. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Re the CodeRabbit docstring-coverage warning: not adding boilerplate docstrings. AGENTS.md asks for comments that describe how a thing is used, and the new module and controls already carry those. No actionable review findings remain on a31f263. |
a31f263 to
ff9540d
Compare
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ff9540d to
5c5641a
Compare
| // Titlebar content clears the sidebar toggle plus the Back and Forward controls. | ||
| ...(isElectron | ||
| ? { | ||
| "--workspace-titlebar-content-left": |
There was a problem hiding this comment.
Tailwind in the owning component: This static titlebar inset belongs in a conditional Tailwind arbitrary-property class on the sidebar provider, rather than in sidebarProviderStyle.
Posted via Macroscope — UI Consistency
|
Review requested
Logged so this PR shows when a maintainer was asked to review it. |
Problem
The desktop app has no Back or Forward buttons. #13212 added
navigation.back/navigation.forward(mod+[/mod+]), but there is nothing to click and no hint the shortcuts exist. Browsers already have their own buttons, so this only affects the desktop window.Change
Adds Back and Forward buttons to the desktop titlebar, right of the sidebar toggle. They do the same thing as the shortcuts, and their tooltips show the shortcut.
useCanGoBack.navigationHistory.tstracks the furthest entry reached since the last push. A new push clears Forward, and replace keeps it. Tracking starts when the router is created. The position is saved in sessionStorage with the current entry's key, so reloading the window keeps Forward; a fresh document ignores it.mainnow wraps the environment pill onto a clipped second line when it does not fit, which covers the same case.)Web is unchanged.
Opening Settings, then clicking Back and Forward (after, c6df7ad). Back enables after the push. After going back, Forward enables and Back disables. Forward then returns to Settings:
More states
Collapsed sidebar, before and after. The header clears all three buttons:
Back tooltip:
840 px window (narrowest sidebar). The brand clips inside the sidebar instead of overlapping the header:
Flow as MP4
Scope and approval
This is the narrower follow-up @juliusmarminge invited when closing #8727 in favor of #13212: "Feel free to reopen a narrower follow-up if you still want chrome back/forward buttons or the mobile header controls from this PR." This PR covers only the desktop titlebar buttons. Mobile header controls are not included.
Verification
vp test run apps/web/src/navigationHistory.test.ts apps/web/src/keybindings.test.ts: 139 passed. The new tests use a real memory history and cover Forward after going back, a push clearing Forward, a replace keeping it, an untracked history, restoring Forward for the same entry after a reload, and ignoring a stored position from another entry.vp run typecheckinapps/webpassed.vp linton the changed files passed, apart from one existingset-state-in-effectwarning inAppSidebarLayout.tsx.vp run dev:desktop, macOS, disposable worktree state with one project and one thread). Driven over the Electron remote-debugging port with real mouse events. Observed: both buttons disabled at start; Back enabled after opening Settings; Forward enabled after Back; Forward disabled again after Forward; tooltip text; collapsed-sidebar layout; 840 px window. Before shots are frommainat 151c241 in the same build and window. On a31f263 I also checked that after Settings → Back → reload, Forward stays enabled and still works.Limits:
Built with Claude Opus 5.5 in Claude Code (via T3 Code). Independent review by Codex GPT-6.1-Sol.